Prepare shared code: Support running on non default dns settings #893

maltesander · 2024-10-16T12:25:51Z

Description

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

# Author
- [x] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Integration tests passed (for non trivial changes)

# Reviewer
- [x] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [x] Changelog updated
- [x] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

…n-default-dns-settings

crates/stackable-operator/CHANGELOG.md

Techassi

Partial review.

Would you mind if I added a bunch of commits to this PR to adjust a few things?

crates/stackable-operator/CHANGELOG.md

crates/stackable-operator/src/utils/cluster_domain.rs

maltesander · 2024-10-16T14:18:46Z

Partial review.

Would you mind if I added a bunch of commits to this PR to adjust a few things?

Just pushed my test fix, did not look at your changes. Dont plan on pushing more today (do a little testing) so feel free to add :)

There is still some bits and pieces we want to change, mostly regarding the error type / error handling. This commit also moves the input fixtures for testing the resolv parser into dedicated files and utilizies rstest's #[files] attribute to generate tests based on these files. Co-authored-by: Nick Larsen <nick.larsen@stackable.tech>

…ttings

Co-authored-by: Techassi <git@techassi.dev>

sbernauer

LGTM otherwise

crates/stackable-operator/src/utils/cluster_domain.rs

Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>

sbernauer

Many thanks, looks good!

NickLarsenNZ

Left some suggestions about error handling. I'm happy to make the changes if you want?

crates/stackable-operator/src/utils/cluster_domain.rs

Co-authored-by: Malte Sander <malte.sander.it@gmail.com> Co-authored-by: Techassi <sascha.lautenschlaeger@stackable.tech>

NickLarsenNZ

LGTM

Already dismissed

nightkr · 2024-10-18T11:10:02Z

crates/stackable-operator/src/client.rs

+    let _ = KUBERNETES_CLUSTER_DOMAIN
+        .set(retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?);
+    create_client(field_manager).await
+}


Why is this a global?

Its supposed to be the new "entry point" for operators. There will be more content in there in the future.

It's basically renaming create_client to initialize_operator to be more expressive. By doing this we enforce that operators need to go throw this function and therefore initialize the OnceLock

It's basically renaming create_client to initialize_operator to be more expressive.

But it.. isn't? It says less about what's happening, nor does it explain why this place would be the entry point.

There will be more content in there in the future.

Such as?

By doing this we enforce that operators need to go throw this function and therefore initialize the OnceLock

This still doesn't explain why it needs to be a OnceLock.

Such as?

I have branch where I warn on an unsupported Kubernetes version. It's just not merged yet because of prioritization. In the future it may also read in the recommended product version or some global config object or whatnot or check if there is an update available.

This still doesn't explain why it needs to be a OnceLock.

Agreed, I didn't try do explain that. Do you have an alternative suggestion how we can make this easy-to-use in operators? It looks very intuitive to me, but I'm not a Rust expert!
In the beginning we had a LazyLock, but @Techassi mentioned that we would panic on e.g. an invalid DomainName being configured, and that the operators should error instead of panic, so we needed to change to a OnceLock.

I have branch where I warn on an unsupported Kubernetes version.

That still sounds like a part of.. creating a k8s client.

Agreed, I didn't try do explain that. Do you have an alternative suggestion how we can make this easy-to-use in operators? It looks very intuitive to me, but I'm not a Rust expert!

I would have said you could store it in the Client, but on second thought maybe it would make more sense to create a separate KubernetesClusterConfig struct.

LazyLock/OnceLock is for caching expensive static values that can't be const for whatever reason (like a precomputed HashMap), not for configuration.

We replaced the LacyLock with a KubernetesClusterInfo struct in #896

nightkr · 2024-10-18T11:11:26Z

crates/stackable-operator/src/utils/cluster_domain.rs

+
+    match env::var(KUBERNETES_SERVICE_HOST_ENV) {
+        Ok(_) => {
+            let cluster_domain = retrieve_cluster_domain_from_resolv_conf(RESOLVE_CONF_FILE_PATH)?;


If we're going to do this hack (and I still don't think we should), can we please at least punt that until vNext instead of sneaking it in just before the release?

We have many customers that require this, i think there is no way to punt on this for now, but calls for intensive testing for sure!

We can punt on autodetection while still keeping the override.

We removed auto-detection in #896 again

maltesander added 4 commits October 15, 2024 15:59

wip

ab8e419

rework - wip

e972aee

docs & clippy fix

96ff314

fix tests

c699b9c

maltesander requested a review from a team October 16, 2024 12:25

maltesander self-assigned this Oct 16, 2024

maltesander mentioned this pull request Oct 16, 2024

Support running on non-default cluster DNS settings stackabletech/issues#436

Closed

2 tasks

maltesander added 2 commits October 16, 2024 14:31

adapted changelog

68f9d6a

fix changelog entry

dd2d672

Techassi self-requested a review October 16, 2024 12:37

maltesander added 3 commits October 16, 2024 14:37

add missing log message

084e1d2

improve logging messages

06b5aac

Merge remote-tracking branch 'origin/main' into support-running-on-no…

b0d3de4

…n-default-dns-settings

sbernauer reviewed Oct 16, 2024

View reviewed changes

crates/stackable-operator/CHANGELOG.md Outdated Show resolved Hide resolved

mark changelog entry as breaking

e053479

sbernauer self-requested a review October 16, 2024 13:51

fix whitespaces

3d5c01e

Techassi previously requested changes Oct 16, 2024

View reviewed changes

fix tests

53d8c9f

Techassi and others added 10 commits October 16, 2024 16:52

chore: Merge branch 'main' into support-running-on-non-default-dns-se…

f7537c7

…ttings

chore: Adjust error variants

d3bd713

chore: Update dev comment

5da95d7

chore: Adjust doc comment

c4b22b5

chore: Adjust changelog

ab93d4e

chore: Add no_run to example

abf10f9

Update crates/stackable-operator/src/utils/cluster_domain.rs

cc570fe

Co-authored-by: Techassi <git@techassi.dev>

improve tracing messages

954bbbc

add note

db8a7c9

maltesander added 2 commits October 17, 2024 10:10

replace expect with error handling

da290fa

clippy

59ca78b

maltesander requested a review from Techassi October 17, 2024 08:56

refactor: Adjust tracing

23697ae

sbernauer previously approved these changes Oct 17, 2024

View reviewed changes

crates/stackable-operator/src/utils/cluster_domain.rs Outdated Show resolved Hide resolved

crates/stackable-operator/src/utils/cluster_domain.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

816b9ef

Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>

maltesander dismissed sbernauer’s stale review via 816b9ef October 17, 2024 11:46

Remove re-export

c90cee8

sbernauer previously approved these changes Oct 17, 2024

View reviewed changes

fix docs

d0ab020

maltesander dismissed sbernauer’s stale review via d0ab020 October 17, 2024 11:57

sbernauer previously approved these changes Oct 17, 2024

View reviewed changes

NickLarsenNZ requested changes Oct 17, 2024

View reviewed changes

chore: Apply suggestions from code review

c686568

Co-authored-by: Malte Sander <malte.sander.it@gmail.com> Co-authored-by: Techassi <sascha.lautenschlaeger@stackable.tech>

NickLarsenNZ dismissed sbernauer’s stale review via c686568 October 17, 2024 13:41

chore: Apply suggestions from code review

e9afa5a

Co-authored-by: Malte Sander <malte.sander.it@gmail.com> Co-authored-by: Techassi <sascha.lautenschlaeger@stackable.tech>

NickLarsenNZ requested review from sbernauer and NickLarsenNZ October 17, 2024 13:47

NickLarsenNZ approved these changes Oct 17, 2024

View reviewed changes

sbernauer approved these changes Oct 17, 2024

View reviewed changes

maltesander added this pull request to the merge queue Oct 17, 2024

Merged via the queue into main with commit 5cf02b5 Oct 17, 2024
10 checks passed

maltesander deleted the support-running-on-non-default-dns-settings branch October 17, 2024 15:48

sbernauer mentioned this pull request Oct 18, 2024

chore: Release stackable-operator 0.79.0 #894

Merged

nightkr reviewed Oct 18, 2024

View reviewed changes

sbernauer mentioned this pull request Oct 22, 2024

refactor!: Don't parse resolv.conf for cluster domain auto-detection #896

Merged

Uh oh!

Prepare shared code: Support running on non default dns settings #893

Prepare shared code: Support running on non default dns settings #893

Uh oh!

Conversation

maltesander commented Oct 16, 2024 • edited by sbernauer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done Checklist

Uh oh!

Uh oh!

Techassi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maltesander commented Oct 16, 2024

Uh oh!

sbernauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbernauer left a comment

Choose a reason for hiding this comment

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightkr Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightkr Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maltesander commented Oct 16, 2024 •

edited by sbernauer

Loading

nightkr Oct 21, 2024 •

edited

Loading

nightkr Oct 21, 2024 •

edited

Loading